[WIP] use machine-config-osimagestream to avoid hard-coding image tag names#10416
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ab08bb0 to
5f7f1bc
Compare
| # Note: Proxy environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) are | ||
| # automatically inherited from the environment if set via /etc/profile.d/proxy.sh | ||
| until COREOS_IMAGE=$("${TEMP_DIR}/machine-config-osimagestream" \ | ||
| get default-node-image \ |
There was a problem hiding this comment.
Well, we don't want the default OS image, we need to return the OS image install-config points too.
We need to pass it by using
We can use this PR to remove the
StreamTag field we won't use anymore.
There was a problem hiding this comment.
I removed the StreamTag field and tried to just use the OSImageStream field. Unless this field is explicitly set in the install-config, it will be empty. I pushed up a change that detects this case and sets it to rhcos.DefaultOSImageStream.
This made me think about what should be the source of truth for what the default OSImageStream is: Should it be the OSImageStream code in the MCO repo? Or should it be the installer code?
We can likely defer that decision for now because the way that both the installer and MCO code is written, it shouldn't matter much. Nevertheless, it is something we should think about.
| --authfile /root/.docker/config.json \ | ||
| --per-host-certs /etc/containers/certs.d \ | ||
| --registry-config /etc/containers/registries.conf); do |
There was a problem hiding this comment.
Can't we rely on the defaults? (Just asking not a blocking question or ask for change)
There was a problem hiding this comment.
In #10406, I don't think we can rely on the defaults since machine-config-osimagestream will be running containerized. However, I think we can rely on the defaults here. I need to look at https://github.com/containers/image to see if there is a SetDefaultValues() function someplace for types.SystemContext. If not, then I'll put these values into the machine-config-osimagestream entrypoint as the default values.
5f7f1bc to
dddbd35
Compare
patrickdillon
left a comment
There was a problem hiding this comment.
What's bad about hardcoding the images? Is it because there are potentially more streams than rhcos9 & rhcos10, and we don't want to teach the installer about all of those?
| # Extract the machine-config-osimagestream binary from the MCO image. This avoids having to munge podman args, paths, env vars, etc. | ||
| podman create --name mco-osimagestream "${MCO_IMAGE}" | ||
| podman cp mco-osimagestream:/usr/bin/machine-config-osimagestream "${TEMP_DIR}/machine-config-osimagestream" | ||
| podman rm mco-osimagestream |
There was a problem hiding this comment.
Can we use podman run via
If so, would be a little cleaner, but I'm not sure we can. Just an idea/suggestion.
There was a problem hiding this comment.
So, I tried to do this in a different PR. While I eventually got it to work, it seemed really brittle.
dddbd35 to
43dd650
Compare
Assisted-By: Claude Sonnet 4.5
43dd650 to
fc89859
Compare
No description provided.